Skip to content

http: close pre-request sockets in closeIdleConnections#63470

Open
semimikoh wants to merge 1 commit into
nodejs:mainfrom
semimikoh:http/close-idle-pre-request
Open

http: close pre-request sockets in closeIdleConnections#63470
semimikoh wants to merge 1 commit into
nodejs:mainfrom
semimikoh:http/close-idle-pre-request

Conversation

@semimikoh
Copy link
Copy Markdown
Contributor

@semimikoh semimikoh commented May 21, 2026

Problem

server.closeIdleConnections() does not close TCP connections that have been accepted but have not yet sent any HTTP data, contradicting its
documented behavior ("Closes all connections connected to this server which are not
sending a request or waiting for a response") and blocking graceful shutdown when peers (e.g. browsers opening speculative connections) hold
sockets open without writing.

Repro:

import { createServer } from 'node:http';
import { createConnection } from 'node:net';

const server = createServer((req, res) => res.end('ok'));
server.listen(3099, '127.0.0.1');
await new Promise((r) => server.once('listening', r));

const sock = createConnection(3099, '127.0.0.1');
await new Promise((r) => sock.once('connect', r));
await new Promise((r) => server.once('connection', r));

const closed = new Promise((r) => server.close(() => r(performance.now())));
const closeStart = performance.now();
server.closeIdleConnections();

const t = await Promise.race([
  closed,
  new Promise((r) => setTimeout(() => r(null), 2000)),
]);
console.log(t === null ? 'still hung after 2s' : `closed in ${t - closeStart}ms`);
sock.destroy();

Before: still hung after 2s. After: closes immediately.

Reported in #63452.

Cause

New sockets are pushed into kConnections from connectionListenerInternal via parser.initialize(...), but Parser::Initialize
(src/node_http_parser.cc:730) sets last_message_start_ = uv_hrtime() so that headersTimeout / requestTimeout can begin counting (DoS
protection). ConnectionsList::Idle() (src/node_http_parser.cc:1147) only returns parsers with last_message_start_ === 0, which becomes true
only after on_message_complete resets it (line 544). Sockets that were accepted but have not sent any HTTP data are therefore classified as active
and skipped by closeIdleConnections().

Fix

lib/_http_server.js: in addition to the existing idle() iteration, walk all() and destroy any socket whose bytesRead === 0. That uniquely
identifies sockets that have not received any data and so cannot be sending a request or awaiting a response. Keep-alive sockets between requests,
in-flight requests, and in-flight responses all have bytesRead > 0 and are unaffected.

Test

Added test/parallel/test-http-server-close-idle-connections-pre-request.js. It opens a TCP connection without sending any data, calls
server.close() + server.closeIdleConnections(), and uses an unref'd setTimeout(common.mustNotCall(), 1000) as the safety net: if the fix works
the event loop drains and the timer never fires; without the fix the timer fires and the assertion trips.

Verification

$ # apply the patch logic as a runtime override and run the repro
$ ./node /tmp/verify_patch.mjs
after closeIdleConnections, awaiting close...
closed in 0.37ms

A full local build of the patched binary was not completed because this machine has Apple clang 16.0.0, while the current tree requires a newer
toolchain (V8 fails to compile due to missing std::atomic_ref). The behavior change was verified by monkey-patching
Server.prototype.closeIdleConnections with the same logic and running the issue's reproduction.

Fixes: #63452

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels May 21, 2026
Signed-off-by: semimikoh <ejffjeosms@gmail.com>
@semimikoh semimikoh force-pushed the http/close-idle-pre-request branch from 2fa5144 to 95c435e Compare May 21, 2026 04:49
@codecov
Copy link
Copy Markdown

codecov Bot commented May 21, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.05%. Comparing base (614050b) to head (95c435e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63470      +/-   ##
==========================================
- Coverage   91.67%   90.05%   -1.62%     
==========================================
  Files         361      714     +353     
  Lines      156408   225925   +69517     
  Branches    24050    42732   +18682     
==========================================
+ Hits       143384   203463   +60079     
- Misses      12751    14238    +1487     
- Partials      273     8224    +7951     
Files with missing lines Coverage Δ
lib/_http_server.js 96.62% <100.00%> (+0.15%) ⬆️

... and 475 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Member

@pimterry pimterry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening this @semimikoh! I'd mildly prefer to do this within ConnectionList itself (see comment) as that'd be a bit cleaner, but I could be persuaded if there's a big issue with that.

Comment thread lib/_http_server.js
@@ -681,6 +681,7 @@ Server.prototype.closeIdleConnections = function closeIdleConnections() {
}

const connections = this[kConnections].idle();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach does look like it works fine, but I wonder if it's changing the wrong level. Could we just make idle() return these connections as well? That would be clearer, and avoid hitting the same issue if we use idle() anywhere else.

I think this is fairly doable: I'd suggest adding a boolean field like received_data_ in the parser, set to false in Initalize, and then true in on_message_begin. Single boolean & write, so very cheap, and then ConnectionsList::Idle can return any connections where it's still false.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

http: Server.closeIdleConnections() does not iterate pre-request sockets

4 participants